Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/core#1108 Fix unsubscribe bug #15815

Merged
merged 2 commits into from
Nov 17, 2019

Conversation

artfulrobot
Copy link
Contributor

Overview

This is detailed at https://lab.civicrm.org/dev/core/issues/1108

Before

Some people would sometimes be unable to unsubscribe due to SQL bugs. Test CRM/Mailing/MailingSystemTest.php has a test for this issue to prove brokenness.

After

Unsubscrbe works again.

@civibot
Copy link

civibot bot commented Nov 11, 2019

(Standard links)


$groups = [];
$base_groups = [];
$mailings = [];

while ($do->fetch()) {
if ($do->entity_table == $group) {
if ($do->entity_table == 'civicrm_group') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this strikes me a potentially problematic

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 why - we save the table as entity_table in that field not the internationalised version don't we?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton nope its the internationalised version posting comment shortly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 ug - we shouldn't - that isn't a pattern anywhere else. We should fix on upgrade

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its been like that for ages, hence why we have put in mulitlingual testing into api/v3/mailing etc. it would be better to sort it long term but that will take some hand holding as there are a number of places in the mailing code where it assumes the internationalised table name is stored

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow - it's an ongoing problem isn't it :-(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still for this PR we can make all the other changes except for that & it will still clean it up a lot

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep agreed and if we make the test changes as per my comment below then we will have test coverage in single and mulitlingual mode for whenever we get around to doing the cleanup work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense for you to do a PR against this branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001
Copy link
Contributor

@artfulrobot @eileenmcnaughton If you make the following changes in your test

diff --git a/tests/phpunit/CRM/Mailing/MailingSystemTest.php b/tests/phpunit/CRM/Mailing/MailingSystemTest.php
index ab1d28c165..049dba67c8 100644
--- a/tests/phpunit/CRM/Mailing/MailingSystemTest.php
+++ b/tests/phpunit/CRM/Mailing/MailingSystemTest.php
@@ -72,6 +72,10 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
   }

   public function tearDown() {
+    global $dbLocale;
+    if ($dbLocale) {
+      CRM_Core_I18n_Schema::makeSinglelingual('en_US');
+    }
     parent::tearDown();
     $this->assertNotEmpty($this->counts['hook_alterMailParams']);
   }
@@ -130,6 +134,10 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
     ], 1);
   }

+  public function multiLingual() {
+    return [[0],[1]];
+  }
+
   /**
    * - unsubscribe used dodgy SQL that only checked half of the polymorphic
    *   relationship in mailing_group, meaning it could match 'mailing 123'
@@ -145,9 +153,10 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
    * - in certain situations (which I have not been able to replicate in this
    *   test) it caused the unsubscribe to fail to find *any* groups to unsubscribe
    *   people from, thereby breaking the unsubscribe.
+   * @dataProvider multiLingual
    *
    */
-  public function testGitLabIssue1108() {
+  public function testGitLabIssue1108($isMultiLingual) {

     // We need to make sure the mailing IDs are higher than the groupIDs.
     // We do this by adding mailings until the mailing.id value is at least 10
@@ -155,6 +164,9 @@ class CRM_Mailing_MailingSystemTest extends CRM_Mailing_BaseMailingSystemTest {
     // Note that creating a row in a transaction then rolling back the
     // transaction still increments the AUTO_INCREMENT counter for the table.
     // (If this behaviour ever changes we throw an exception.)
+    if ($isMultiLingual) {
+      $this->enableMultilingual();
+    }
     $max_group_id = CRM_Core_DAO::singleValueQuery("SELECT MAX(id) FROM civicrm_group");
     $max_mailing_id = 0;
     while ($max_mailing_id < $max_group_id + 10) {

you will note that when it enables the Multilingual it fails the test because we store the internationalised name of the table in the database in entity_table because reasons

So the way i fixed it locally was applying this

diff --git a/CRM/Mailing/Event/BAO/Unsubscribe.php b/CRM/Mailing/Event/BAO/Unsubscribe.php
index 1bc8995138..36499300e4 100644
--- a/CRM/Mailing/Event/BAO/Unsubscribe.php
+++ b/CRM/Mailing/Event/BAO/Unsubscribe.php
@@ -143,6 +143,10 @@ WHERE  email = %2
     $mailing_id = civicrm_api3('MailingJob', 'getvalue', ['id' => $job_id, 'return' => 'mailing_id']);
     $mailing_type = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_Mailing', $mailing_id, 'mailing_type', 'id');

+    $groupObject = new CRM_Contact_BAO_Group();
+    $group = $groupObject->getTableName();
diff --git a/CRM/Mailing/Event/BAO/Unsubscribe.php b/CRM/Mailing/Event/BAO/Unsubscribe.php
index 1bc8995138..36499300e4 100644
--- a/CRM/Mailing/Event/BAO/Unsubscribe.php
+++ b/CRM/Mailing/Event/BAO/Unsubscribe.php
@@ -143,6 +143,10 @@ WHERE  email = %2
     $mailing_id = civicrm_api3('MailingJob', 'getvalue', ['id' => $job_id, 'return' => 'mailing_id']);
     $mailing_type = CRM_Core_DAO::getFieldValue('CRM_Mailing_DAO_Mailing', $mailing_id, 'mailing_type', 'id');

+    $groupObject = new CRM_Contact_BAO_Group();
+    $group = $groupObject->getTableName();
+    $mailingObject = new CRM_Mailing_BAO_Mailing();
+    $mailing = $mailingObject->getTableName();
     // We need a mailing id that points to the mailing that defined the recipients.
     // This is usually just the passed-in mailing_id, however in the case of AB
     // tests, it's the variant 'A' one.
@@ -185,7 +189,7 @@ WHERE  email = %2
     $mailings = [];

     while ($do->fetch()) {
-      if ($do->entity_table == 'civicrm_group') {
+      if ($do->entity_table === $group) {
         if ($do->group_type == 'Base') {
           $base_groups[$do->entity_id] = NULL;
         }
@@ -193,7 +197,7 @@ WHERE  email = %2
           $groups[$do->entity_id] = NULL;
         }
       }
-      elseif ($do->entity_table == 'civicrm_mailing') {
+      elseif ($do->entity_table === $mailing) {
         $mailings[] = $do->entity_id;
       }
     }
@@ -212,10 +216,10 @@ WHERE  email = %2
       $mailings = [];

       while ($do->fetch()) {
-        if ($do->entity_table == 'civicrm_group') {
+        if ($do->entity_table === $group) {
           $groups[$do->entity_id] = TRUE;
         }
-        elseif ($do->entity_table == 'civicrm_mailing') {
+        elseif ($do->entity_table === $mailing) {
           $mailings[] = $do->entity_id;
         }
       }

@artfulrobot
Copy link
Contributor Author

Hee hee, so not completely, but nearly back to what it was! Ok, I can apply those changes. At CiviCamp London tomorrow, not sure when I'll get time.

@eileenmcnaughton
Copy link
Contributor

@artfulrobot right so use the variable when it's saved to the table (value in entiy_table) because ... yuck. But use the table name in the FROM clause

@artfulrobot
Copy link
Contributor Author

@seamuslee001 thanks for showing me the multilingual stuff.

Seems like it might have been a better decision to stick to a single language for table names (as we have to do for the code, for example). But I do say that as a privileged English speaker.

Anyway, resubmitted the 2 commits. I didn't squah them so that if needed someone could run the tests agains the old code, but let me know if you want them squishing.

@mlutfy mlutfy changed the title fix unsubscribe bug Issue #1108 dev/core#1108 Fix unsubscribe bug Nov 15, 2019
@eileenmcnaughton
Copy link
Contributor

@seamuslee001 is this good to go now?

@seamuslee001
Copy link
Contributor

Yes and when AUG last had issues Jitendra added a unit test also which is here https://github.com/civicrm/civicrm-core/pull/10165/files so that verifies that this doesn't break that so i'm good with this now merging

@seamuslee001 seamuslee001 merged commit 5ec753e into civicrm:master Nov 17, 2019
@artfulrobot
Copy link
Contributor Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants